Bump min go version to 1.18 && read build info from embedded binary #2548

dprotaso · 2022-07-16T01:48:21Z

🧹 Bump go version to 1.18
🧹 Read commit info from embedded binary
- https://pkg.go.dev/runtime/debug#ReadBuildInfo
- https://tip.golang.org/doc/go1.18#go-command

This should let us drop HEAD and refs in our kodata folders

/kind cleanup

Release Note

changeset.Get now reads from vcs information embedded by the golang compiler

dprotaso · 2022-07-16T01:48:48Z

cc @wlynch

codecov · 2022-07-16T02:08:41Z

Codecov Report

Merging #2548 (512c6b4) into main (418e138) will decrease coverage by 0.02%.
The diff coverage is 97.36%.

@@            Coverage Diff             @@
##             main    #2548      +/-   ##
==========================================
- Coverage   81.64%   81.61%   -0.03%     
==========================================
  Files         163      163              
  Lines        9745     9715      -30     
==========================================
- Hits         7956     7929      -27     
+ Misses       1551     1549       -2     
+ Partials      238      237       -1

Impacted Files	Coverage Δ
logging/config.go	`81.25% <75.00%> (ø)`
apis/contexts.go	`100.00% <100.00%> (ø)`
changeset/commit.go	`100.00% <100.00%> (+4.83%)`	⬆️
...k/resourcesemantics/validation/validation_admit.go	`93.12% <100.00%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1f36a2...512c6b4. Read the comment docs.

dprotaso · 2022-07-16T05:25:26Z

unit tests flaked (metrics)

cc @imjasonh for tekton pipeline downstream failure -

dprotaso · 2022-07-16T05:27:00Z

One option is we fail silently if the vcs.revision is missing - which would happen in go test scenarios or go run somefile.go

imjasonh · 2022-07-16T06:22:37Z

One option is we fail silently if the vcs.revision is missing - which would happen in go test scenarios or go run somefile.go

Or if the binaries/images are built with -buildvcs=false, which some downstream might want to do if, e.g., Git isn't available in the CI environment, or they want to omit source information for some other reason.

I'll prepare a PR to fix Tekton's usage though, it doesn't look like these tests need to use changeset.Get anyway.

knative/pkg#2548 changes pkg/changeset.Get to read VCS information from information embedded by the Go compiler into built binaries, instead of relying on the convention that a kodata/HEAD symlink exists pointing to the repo's .git/HEAD, and a kodata/refs points to .git/refs. This fails in tests though, since tests don't stamp this information into the binary where it can be read. This change removes our tests' usage of changeset.Get and instead replaces those fake commit SHAs with more obviously placeholder values. It doesn't seem that the tests cared that it was an actual SHA, let along the actual current SHA, they just needed any value that looked like a SHA.

changeset/commit.go

- Use a sync.Once to prevent double parsing - Support SHA256 - Allow non-SHA revision strings

dprotaso · 2022-07-16T15:30:14Z

Ok added some tweaks

Use a sync.Once to prevent double parsing
Support SHA256 (git)
Allow non-SHA revision strings (ie. support SVN or whatever)

imjasonh · 2022-07-18T14:57:17Z

changeset/commit.go

-	koDataPath := os.Getenv(koDataPathEnvName)
-	if koDataPath == "" {
-		return nil, fmt.Errorf("%q does not exist or is empty", koDataPathEnvName)
+	if shaRegexp.MatchString(revision) {


Technically, we don't need this check 😎

(not necessarily advice to remove it, just a funny coincidence)

Actually, I wonder if this should just be if len(revision) >= 7, so folks using Subversion or whatever can still enjoy this goodness. I'm not sure there aren't other places we assume Git in and around here, but just in case.

I think I'd leave it as - since I don't want to assume truncating will work for different VCS - ie svn is numeric so we'd be alternating the commit id

A further improvement could be to do this only for vcs that use SHA's for revisions -ie git, hg etc.

knative-prow · 2022-07-18T14:58:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, imjasonh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

imjasonh · 2022-07-18T15:29:36Z

changeset/commit.go

+// Get returns the 'vcs.revision' property from the embedded build information
+// This function will return an error if value is not a valid Git SHA
+//
+// The result will have a '-dirty' suffix if the workspace was not clean
 func Get() (string, error) {


Can we have a changeset.MustGet that panics if it can't detect the version? I'm updating some callsites to get this once at init-time, and they're all just log.Fataling if this returns an error. Or should this return Unknown if it can't read build info, and just never return an error?

Or should this return Unknown if it can't read build info, and just never return an error?

I was thinking about this - let me do it in a follow up

build info (vcs.revision) is now embedded in the go binary with go1.18 see: knative/pkg#2548

knative/pkg#2548 changes pkg/changeset.Get to read VCS information from information embedded by the Go compiler into built binaries, instead of relying on the convention that a kodata/HEAD symlink exists pointing to the repo's .git/HEAD, and a kodata/refs points to .git/refs. This fails in tests though, since tests don't stamp this information into the binary where it can be read. This change removes our tests' usage of changeset.Get and instead replaces those fake commit SHAs with more obviously placeholder values. It doesn't seem that the tests cared that it was an actual SHA, let along the actual current SHA, they just needed any value that looked like a SHA.

dprotaso added 2 commits July 15, 2022 21:43

Bump min go version to 1.18 & read build SHA from binary

9ff63bf

GitHub Commit ID is really a Git Revision

d1a74c1

dprotaso added 2 commits July 15, 2022 22:01

update go.sum

041b3d8

fix spelling

9bd6c91

imjasonh mentioned this pull request Jul 16, 2022

Prepare tests for impending knative/pkg version bump tektoncd/pipeline#5145

Merged

7 tasks

imjasonh reviewed Jul 16, 2022

View reviewed changes

changeset/commit.go Show resolved Hide resolved

Address PR feedback

e1dbd0a

- Use a sync.Once to prevent double parsing - Support SHA256 - Allow non-SHA revision strings

Don't include 'unknown' revision value in logs

512c6b4

imjasonh approved these changes Jul 18, 2022

View reviewed changes

knative-prow bot assigned imjasonh Jul 18, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2022

knative-prow bot merged commit 473ba31 into knative:main Jul 18, 2022

dprotaso deleted the changeset-runtime branch July 18, 2022 15:00

imjasonh reviewed Jul 18, 2022

View reviewed changes

dprotaso mentioned this pull request Jul 18, 2022

Drop error from changeset.Get signature #2549

Merged

dprotaso added a commit to dprotaso/serving that referenced this pull request Jul 18, 2022

Don't included git info in our containers

b6267cd

build info (vcs.revision) is now embedded in the go binary with go1.18 see: knative/pkg#2548

dprotaso mentioned this pull request Jul 18, 2022

Remove git files from kodata knative/serving#13130

Merged

knative-prow bot pushed a commit to knative/serving that referenced this pull request Jul 18, 2022

Don't included git info in our containers (#13130)

017b9d0

build info (vcs.revision) is now embedded in the go binary with go1.18 see: knative/pkg#2548

psschwei mentioned this pull request Jul 21, 2022

Bump go version in kind-e2e tests to v1.18 knative/serving#13140

Merged

matzew mentioned this pull request Aug 4, 2022

use go 1.18 knative/eventing#6470

Merged

imjasonh mentioned this pull request Aug 11, 2022

Determine changeset from build information tektoncd/pipeline#5311

Merged

7 tasks

psschwei mentioned this pull request Sep 30, 2022

Gate knative-extensions/security-guard#57

Merged

khrm mentioned this pull request Feb 22, 2023

Remove git files from KODATA tektoncd/triggers#1533

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump min go version to 1.18 && read build info from embedded binary #2548

Bump min go version to 1.18 && read build info from embedded binary #2548

dprotaso commented Jul 16, 2022

dprotaso commented Jul 16, 2022

codecov bot commented Jul 16, 2022 •

edited

Loading

dprotaso commented Jul 16, 2022

dprotaso commented Jul 16, 2022 •

edited

Loading

imjasonh commented Jul 16, 2022

dprotaso commented Jul 16, 2022 •

edited

Loading

imjasonh Jul 18, 2022

dprotaso Jul 18, 2022

dprotaso Jul 18, 2022

knative-prow bot commented Jul 18, 2022

imjasonh Jul 18, 2022

dprotaso Jul 18, 2022

Bump min go version to 1.18 && read build info from embedded binary #2548

Bump min go version to 1.18 && read build info from embedded binary #2548

Conversation

dprotaso commented Jul 16, 2022

dprotaso commented Jul 16, 2022

codecov bot commented Jul 16, 2022 • edited Loading

Codecov Report

dprotaso commented Jul 16, 2022

dprotaso commented Jul 16, 2022 • edited Loading

imjasonh commented Jul 16, 2022

dprotaso commented Jul 16, 2022 • edited Loading

imjasonh Jul 18, 2022

Choose a reason for hiding this comment

dprotaso Jul 18, 2022

Choose a reason for hiding this comment

dprotaso Jul 18, 2022

Choose a reason for hiding this comment

knative-prow bot commented Jul 18, 2022

imjasonh Jul 18, 2022

Choose a reason for hiding this comment

dprotaso Jul 18, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 16, 2022 •

edited

Loading

dprotaso commented Jul 16, 2022 •

edited

Loading

dprotaso commented Jul 16, 2022 •

edited

Loading